Skip to content

Fix issue with handling of failure during discard of metadata cache entries #5817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2025
Merged

Fix issue with handling of failure during discard of metadata cache entries #5817

merged 1 commit into from
Oct 16, 2025

Conversation

@jhendersonHDF
Copy link
Collaborator

@jhendersonHDF jhendersonHDF commented Sep 14, 2025 •
edited
Loading

When discarding a metadata cache entry after flushing it, errors during the discard process could cause the library to skip calling the 'free_icr' callback for the entry. This could result in resource leaks and the inability of the cache to be fully flushed and closed due to issues such as pinned entries remaining in the cache. This has been fixed by noting errors during the discard process, but attempting to fully free a cache entry before signalling that an error has occurred.

Fixes CVE-2025-7068

Fixes #5578
Also fixes #4586

Important

Fixes resource leak issue in metadata cache discard process, addressing CVE-2025-7068 by ensuring proper cleanup in H5Centry.c.

  • Behavior:
    • Fixes issue in H5C__flush_single_entry() in H5Centry.c where errors during discard could skip free_icr callback, causing resource leaks.
    • Introduces H5C__discard_single_entry() to handle entry discard, ensuring cleanup even on errors.
  • Security:
    • Fixes CVE-2025-7068, preventing resource leaks and ensuring cache can be fully flushed and closed.
  • Documentation:

This description was created by Ellipsis for 749a292. You can customize this summary. It will automatically update as commits are pushed.

@jhendersonHDF jhendersonHDF added the Component - C Library Core C library issues (usually in the src directory) label Sep 14, 2025
@github-project-automation github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK Sep 14, 2025
ellipsis-dev[bot]
ellipsis-dev bot reviewed Sep 14, 2025
View reviewed changes
@jhendersonHDF
Copy link
Collaborator Author

jhendersonHDF commented Sep 14, 2025 •
edited
Loading

Context:
When opening a corrupted HDF5 file with read-write access, the process of reading the file superblock checks if a message exists inside the superblock extension object header using H5O_msg_exists, which causes H5O_protect to load an object header continuation chunk into the cache and deserialize it, increasing the reference count on the superblock extension object header and pinning it in the cache as part of H5O__inc_rc. A bit later during the file superblock reading process, the library finds a message in the superblock extension indicating that there is a metadata cache image to load and calls H5AC_load_cache_image_on_next_protect to have the metadata cache load it on the next H5C_protect call. When attempting to open a dataset in the corrupted file, the library eventually tries to load the cache image as part of H5G__obj_lookup, but fails. During the process of trying to load the cache image, the library is unable to delete the cache image by removing the cache image message from the superblock extension due to corruption in the file free space manager. When the metadata cache fails to free the file space for the relevant object header chunk, it throws an error and skips over calling the free_icr callback for the chunk, eventually leaving the superblock extension object header pinned in the cache with an increased reference count. When the file is closed later on, the superblock extension object header can't be flushed, leaking the memory for the chunk proxy structure allocated for the continuation chunk during H5O_protect.

@jhendersonHDF jhendersonHDF marked this pull request as ready for review September 14, 2025 01:26
mattjala
mattjala previously approved these changes Sep 16, 2025
View reviewed changes
mattjala
mattjala previously approved these changes Sep 17, 2025
View reviewed changes
@nbagha1 nbagha1 moved this from To be triaged to Scheduled/On-Deck in HDF5 - TRIAGE & TRACK Sep 23, 2025
fortnern
fortnern reviewed Sep 23, 2025
View reviewed changes
src/H5Centry.c
if (entry_ptr->type->fsf_size) {
if ((entry_ptr->type->fsf_size)((void *)entry_ptr, &fsf_size) < 0)
/* Note error but keep going */
HDONE_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "unable to get file space free size");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure fsf_size is set to 0 if this fails, since it gets passed to H5MM_xfree. Either set it here or initialize it to 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply skip calling H5MM_xfree if fsf_size fails

Copy link
Collaborator Author

@jhendersonHDF jhendersonHDF Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the call to H5MF_xfree below? If so, I added the (ret_value >= 0) for that case. A little hacky, but figured it was best to skip.

fortnern
fortnern requested changes Sep 23, 2025
View reviewed changes
Copy link
Member

@fortnern fortnern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment but looks good otherwise

@github-project-automation github-project-automation bot moved this from Scheduled/On-Deck to In progress in HDF5 - TRIAGE & TRACK Sep 23, 2025
@nbagha1 nbagha1 added this to the Release 2.0.0 milestone Sep 26, 2025
mattjala
mattjala previously approved these changes Sep 26, 2025
View reviewed changes
bmribler
bmribler reviewed Oct 6, 2025
View reviewed changes
src/H5Centry.c
* it to indicate that it was removed.
*/
cache_ptr->entries_removed_counter++;
cache_ptr->last_entry_removed_ptr = entry_ptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance cache_ptr->last_entry_removed_ptr is pointing to something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to say no, but I didn't validate any of the comments here.

bmribler
bmribler requested changes Oct 6, 2025
View reviewed changes
Copy link
Collaborator

@bmribler bmribler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comments

@brtnfld
Copy link
Collaborator

brtnfld commented Oct 8, 2025

@fortnern @bmribler @mattjala, can we get this approved as soon as possible, as it should resolve other CVE issues? If you can not approve it, please be clear as to why.

bmribler
bmribler previously approved these changes Oct 9, 2025
View reviewed changes
@jhendersonHDF
Fix issue with handling of failure during discard of metadata cache e…
58f5a6b 
…ntries

When discarding a metadata cache entry after flushing it, errors
during the discard process could cause the library to skip calling
the 'free_icr' callback for the entry. This could result in resource
leaks and the inability of the cache to be fully flushed and closed
due to issues such as pinned entries remaining in the cache. This
has been fixed by noting errors during the discard process, but
attempting to fully free a cache entry before signalling that an
error has occurred.

Fixes CVE-2025-7068
@jhendersonHDF jhendersonHDF requested a review from bmribler October 9, 2025 17:50
@bmribler
Copy link
Collaborator

@fortnern @mattjala Could you please re-review and re-approve this when you get the chance?

@lrknox lrknox merged commit 7dd1102 into HDFGroup:develop Oct 16, 2025
90 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in HDF5 - TRIAGE & TRACK Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

@fortnern fortnern fortnern approved these changes

@bmribler bmribler bmribler approved these changes

@mattjala mattjala mattjala approved these changes

@lrknox lrknox Awaiting requested review from lrknox lrknox is a code owner

@derobins derobins Awaiting requested review from derobins derobins is a code owner

@byrnHDF byrnHDF Awaiting requested review from byrnHDF byrnHDF is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

Assignees

@fortnern fortnern

@bmribler bmribler

Labels

Component - C Library Core C library issues (usually in the src directory)

Projects

HDF5 - TRIAGE & TRACK
Status: Done

Milestone

Release 2.0.0

Development

Successfully merging this pull request may close these issues.

[BUG] Memory leaks in H5FL__malloc at src/H5FL.c:211:30 A memory leak was found while testing h5_extended_fuzzer with libfuzzer

8 participants

@jhendersonHDF @brtnfld @bmribler @fortnern @mattjala @lrknox @vchoi-hdfgroup @nbagha1